-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use external concept counts #1136
Conversation
…s not already exist. Skip results datamode test if SKIP_DB_TESTS is TRUE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor changes suggested - add when ready.
A possible change if you want would be to make the CD concept counts table here identical to the fields needed in the achilles table - before I go and check @ablack3 @cebarboza, are they substantially different?
R/RunDiagnostics.R
Outdated
conceptCountsTable <- "#concept_counts" | ||
} | ||
} else { | ||
if (conceptCountsTable == "#concept_counts") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be substr(conceptCountsTable, 1, 1) == "#"
as otherwise #my_temp_table
will render as work_database_schema.#my_temp_table
which will crash on some systems
R/RunDiagnostics.R
Outdated
snakeCaseToCamelCase = TRUE, | ||
tempEmulationSchema = getOption("sqlRenderTempEmulationSchena") | ||
) | ||
if (!identical(vocabVersion, vocabVersionExternalConceptCountsTable[1,1])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great check!
tests/testthat/test-other.R
Outdated
if (dbms == "sqlite") { | ||
# Creating externalConceptCounts | ||
sql_lite_path <- file.path(test_path(), databaseFile) | ||
connectionDetails <- createConnectionDetails(dbms= "sqlite", server = sql_lite_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to build tests out for real databases but this can likely wait - I think adding a pre-computed counts table should speed up the other unit tests
The achilles table we would like to use has "concept_id", "record_count", "descendant_record_count", and we would like to add "person_count" and "descendant_person_count" so we could use those column names. |
We are adding two parameters that depend on each other: Let's list out the expected behavior:
I'm wondering if we could do this with just one parameter, the cohortCountsTable. if it exists we use it and if not we recreate it. |
@cebarboza @ablack3 I'm going to release 3.3.0 today for the HADES wide lock file so this will make its way in to a 3.4.0 release whenever it becomes ready |
Moving this PR to the OHDSI organization so we can run all the tests in github actions.